[`flake8-pyi`] Implement autofix for `redundant-numeric-union` (`PYI041`) #14273

sbrugman · 2024-11-11T12:35:52Z

Summary

This PR adds autofix for redundant-numeric-union (PYI041)

There are some comments below to explain the reasoning behind some choices that might help review.

Resolves part of #14185.

Test Plan

…41`)

crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs

github-actions · 2024-11-11T12:49:52Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -4 violations, +66 -0 fixes in 3 projects; 51 projects unchanged)

apache/airflow (+0 -0 violations, +58 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- airflow/decorators/__init__.pyi:117:25: PYI041 Use `float` instead of `int | float`
+ airflow/decorators/__init__.pyi:117:25: PYI041 [*] Use `float` instead of `int | float`
- airflow/decorators/__init__.pyi:256:25: PYI041 Use `float` instead of `int | float`
+ airflow/decorators/__init__.pyi:256:25: PYI041 [*] Use `float` instead of `int | float`
- airflow/jobs/job.py:308:39: PYI041 Use `float` instead of `int | float`
+ airflow/jobs/job.py:308:39: PYI041 [*] Use `float` instead of `int | float`
- airflow/metrics/base_stats_logger.py:39:15: PYI041 Use `float` instead of `int | float`
+ airflow/metrics/base_stats_logger.py:39:15: PYI041 [*] Use `float` instead of `int | float`
- airflow/metrics/base_stats_logger.py:50:15: PYI041 Use `float` instead of `int | float`
+ airflow/metrics/base_stats_logger.py:50:15: PYI041 [*] Use `float` instead of `int | float`
- airflow/metrics/base_stats_logger.py:61:15: PYI041 Use `float` instead of `int | float`
+ airflow/metrics/base_stats_logger.py:61:15: PYI041 [*] Use `float` instead of `int | float`
- airflow/metrics/datadog_logger.py:103:16: PYI041 Use `float` instead of `int | float`
+ airflow/metrics/datadog_logger.py:103:16: PYI041 [*] Use `float` instead of `int | float`
- airflow/metrics/otel_logger.py:237:16: PYI041 Use `float` instead of `int | float`
+ airflow/metrics/otel_logger.py:237:16: PYI041 [*] Use `float` instead of `int | float`
- airflow/metrics/statsd_logger.py:117:16: PYI041 Use `float` instead of `int | float`
+ airflow/metrics/statsd_logger.py:117:16: PYI041 [*] Use `float` instead of `int | float`
- airflow/utils/timezone.py:240:26: PYI041 Use `float` instead of `int | float`
+ airflow/utils/timezone.py:240:26: PYI041 [*] Use `float` instead of `int | float`
- airflow/utils/timezone.py:305:16: PYI041 Use `float` instead of `int | float`
+ airflow/utils/timezone.py:305:16: PYI041 [*] Use `float` instead of `int | float`
- performance/src/performance_dags/performance_dag/performance_dag_utils.py:175:27: PYI041 Use `float` instead of `int | float`
+ performance/src/performance_dags/performance_dag/performance_dag_utils.py:175:27: PYI041 [*] Use `float` instead of `int | float`
- performance/src/performance_dags/performance_dag/performance_dag_utils.py:337:31: PYI041 Use `float` instead of `int | float`
+ performance/src/performance_dags/performance_dag/performance_dag_utils.py:337:31: PYI041 [*] Use `float` instead of `int | float`
- providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:273:16: PYI041 Use `float` instead of `int | float`
+ providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:273:16: PYI041 [*] Use `float` instead of `int | float`
- providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:302:56: PYI041 Use `float` instead of `int | float`
+ providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:302:56: PYI041 [*] Use `float` instead of `int | float`
- providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:325:57: PYI041 Use `float` instead of `int | float`
+ providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:325:57: PYI041 [*] Use `float` instead of `int | float`
- providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:516:27: PYI041 Use `float` instead of `int | float`
+ providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:516:27: PYI041 [*] Use `float` instead of `int | float`
- providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:516:47: PYI041 Use `float` instead of `int | float`
+ providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:516:47: PYI041 [*] Use `float` instead of `int | float`
- providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:516:72: PYI041 Use `float` instead of `int | float`
+ providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:516:72: PYI041 [*] Use `float` instead of `int | float`
- providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:544:22: PYI041 Use `float` instead of `int | float`
+ providers/src/airflow/providers/amazon/aws/hooks/batch_client.py:544:22: PYI041 [*] Use `float` instead of `int | float`
- providers/src/airflow/providers/amazon/aws/hooks/batch_waiters.py:195:16: PYI041 Use `float` instead of `int | float`
... 17 additional changes omitted for project

apache/superset (+0 -0 violations, +8 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- superset/daos/query.py:68:52: PYI041 Use `float` instead of `int | float`
+ superset/daos/query.py:68:52: PYI041 [*] Use `float` instead of `int | float`
- superset/utils/cache.py:150:14: PYI041 Use `float` instead of `int | float`
+ superset/utils/cache.py:150:14: PYI041 [*] Use `float` instead of `int | float`
- superset/utils/core.py:349:24: PYI041 Use `float` instead of `int | float`
+ superset/utils/core.py:349:24: PYI041 [*] Use `float` instead of `int | float`
- superset/utils/decorators.py:163:24: PYI041 Use `float` instead of `int | float`
+ superset/utils/decorators.py:163:24: PYI041 [*] Use `float` instead of `int | float`

python/typeshed (+0 -4 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select E,F,FA,I,PYI,RUF,UP,W

- stdlib/random.pyi:45:31: PYI041 Use `float` instead of `int | float`
- stdlib/random.pyi:52:27: PYI041 Use `float` instead of `int | float`
- stdlib/turtle.pyi:443:16: PYI041 Use `float` instead of `int | float`
- stdlib/turtle.pyi:444:17: PYI041 Use `float` instead of `int | float`

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PYI041	70	0	4	66	0

crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs

MichaReiser

Thanks. This overall looks great.

The main open questions are

should use text based edits to avoid reformatting unchanged code and loosing inner comments.
Whether it's possible to always mark the fix as safe, regardless of comments

crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs

MichaReiser · 2024-11-12T08:56:43Z

Oh, and did you look through the ecosystem changes? If so, could you update the test plan. It's helpful to know if you did because I then don't need to carefully review all of them and can instead just click through some of them.

sbrugman · 2024-11-12T15:34:32Z

Oh, and did you look through the ecosystem changes? If so, could you update the test plan. It's helpful to know if you did because I then don't need to carefully review all of them and can instead just click through some of them.

The ecosystem results look good.
From this example I'm wondering if we shouldn't include return types; it appears to skip that one now.

Typeshed shows negative in the ecosystem results, but I expect that to not be related to the PR.
~~Maybe @AlexWaygood can help out with that.~~
It's probably because the example is Py3.13+

MichaReiser · 2024-11-13T09:11:29Z

From this example I'm wondering if we shouldn't include return types; it appears to skip that one now.

Could you expand what you mean by it appears to skip that now. Is it not flagging this case or do you think this case shouldn't be flagged?

Is it worth to add a test like that example (or that specific example) to our test suite?

MichaReiser · 2024-11-13T09:14:12Z

crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs

+/// This rule's fix is marked as safe for most cases; however, the fix will
+/// flatten nested unions type expressions into a single top-level union.


Reading the documentation, I get the impression the fix isn't safe because it flattens nested union expressions. But I understand that this doesn't make the fix unsafe, it's just an extension where the fix goes beyond what one might expect.

Exactly. I've reused the phrasing that Charlie used for another rule.

The nesting is a feature to allow cases such as these:

ReadOnlyMode = Literal["r", "r+"] WriteAndTruncateMode = Literal["w", "w+", "wt", "w+t"] WriteNoTruncateMode = Literal["r+", "r+t"] AppendMode = Literal["a", "a+", "at", "a+t"] AllModes = Literal[ReadOnlyMode, WriteAndTruncateMode, WriteNoTruncateMode, AppendMode]

As a consequence this redundant nesting is also allowed, but can be safely fixed:

AllModes = Literal[Literal["r", "r+"], Literal["w", "w+", "wt", "w+t"], Literal["r+", "r+t"], Literal["a", "a+", "at", "a+t"]]

See https://typing.readthedocs.io/en/latest/spec/literal.html#legal-and-illegal-parameterizations

~~We probably should have~~ I will create a rule for this (for both union and literal).

After reading the linked document, I understand that both examples are valid. Can you share an example where flattening a nested union is unsafe?

I tweaked the comment a bit on all three rules.

crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs

…c_union.rs Co-authored-by: Micha Reiser <micha@reiser.io>

AlexWaygood · 2024-11-13T11:28:15Z

Typeshed shows negative in the ecosystem results, but I expect that to not be related to the PR.
~~Maybe @AlexWaygood can help out with that.~~
It's probably because the example is Py3.13+

The ecosystem reports for typeshed have often not been reproducible locally and nobody's yet had a chance to look into why... if you can't repro the typeshed hits locally, I'd just ignore them!

sbrugman · 2024-11-13T14:53:37Z

Could you expand what you mean by it appears to skip that now. Is it not flagging this case or do you think this case shouldn't be flagged?

After investigating this, it turned out that this rule hooked into statement.rs instead of expression.rs like all the other rules related to unions do. This caused false negatives on the return types and type aliases. We had no test for this. Fixed it, and added a test.

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

…41`) (astral-sh#14273) ## Summary This PR adds autofix for `redundant-numeric-union` (`PYI041`) There are some comments below to explain the reasoning behind some choices that might help review.  Resolves part of astral-sh#14185. ## Test Plan  --------- Co-authored-by: Micha Reiser <micha@reiser.io> Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>

[flake8-pyi] Implement autofix for redundant-numeric-union (`PYI0…

6baf63d

…41`)

sbrugman requested a review from AlexWaygood as a code owner November 11, 2024 12:35